-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement ExecuteDelete #2449
Implement ExecuteDelete #2449
Conversation
5e877b9
to
08aebff
Compare
{ | ||
table = selectExpression.Tables[0]; | ||
} | ||
else if (selectExpression.Tables.All(t => t is TableExpression or InnerJoinExpression { Table: TableExpression })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitpatel note this filter to only accept inner joins over tables (since that's what PG USING does/supports).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little sad that only inner join is supported. I will add test which uses multiple inner joins. What do you think about cross apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree it's sad... Good to add some more tests (multiple inner joins, left/cross...).
I'll take a look at whether USING allows cross apply (lateral join as PG calls it). My gut says no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitpatel unfortunately cross apply (lateral join) doesn't work with DELETE's USING:
DELETE FROM "Order Details" AS o
USING (
SELECT o0."OrderID", o0."CustomerID", o0."EmployeeID", o0."OrderDate"
FROM "Orders" AS o0
WHERE o0."OrderID" < o."OrderID"
ORDER BY o0."OrderID" NULLS FIRST
LIMIT 100 OFFSET 0
) AS t
WHERE o."OrderID" < 10276
This errors with:
42P01: invalid reference to FROM-clause entry for table "o"
Note that it is apparently possible to do it with UPDATE's FROM, as here:
UPDATE x
SET foo=bar
FROM a, LATERAL (...)
I tried putting LATERAL betwen the subquery and USING in the DELETE above and that didn't work either 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work in id in subquery form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the default behavior with the PK comparison? Sure, here's your test which passes. It's just that DELETE"s USING clause is apparently only for inner joins...
test/EFCore.PG.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesNpgsqlTest.cs
Show resolved
Hide resolved
test/EFCore.PG.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesNpgsqlTest.cs
Show resolved
Hide resolved
ac0c01c
to
280ea15
Compare
/// Converts the relational <see cref="NonQueryExpression" /> into a PG-specific <see cref="PostgresDeleteExpression" />, which | ||
/// precisely models a DELETE statement in PostgreSQL. This is done to handle the PG-specific USING syntax for table joining. | ||
/// </summary> | ||
public class NonQueryConvertingExpressionVisitor : ExpressionVisitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitpatel here's the conversion to the PG-specific delete expression, as discussed in #2449 (comment). I think it does make it nicer, and query SQL generation is now trivial as it should be. Any thoughts?
src/EFCore.PG/Query/Expressions/Internal/PostgresDeleteExpression.cs
Outdated
Show resolved
Hide resolved
test/EFCore.PG.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesNpgsqlTest.cs
Show resolved
Hide resolved
Following dotnet/efcore#28492 Part of npgsql#2450
Synced against dotnet/efcore#28492.
Nothing much to see here yet, the tests are implemented and all pass (see last commit only). The PostgreSQL-specific USING syntax for joining needs to be implemented, will do that following upstream implementation for SQL Server.
Closes #2450
/cc @smitpatel